Fix for issue with initial-input in org-ql-view--complete-buffers-files (Fix #227) #228
Conversation
|
Which values have you tested this with? |
|
For me the issues was when using the prefix argument for refreshing while in the org-ql-view/agenda buffer. I've tested this with different org file and queries and with selectrum and the vanilla completing-read. Is that what you were looking for? I am not familiar with writing tests on elisp. |
|
An alternative is to use the (defun org-ql-view--complete-buffers-files ()
"Return value for `org-ql-view-buffers-files' using completion."
(cl-labels ((initial-input
() (when org-ql-view-buffers-files
(org-ql-view--contract-buffers-files
org-ql-view-buffers-files))))
(if (and org-ql-view-buffers-files
(bufferp org-ql-view-buffers-files))
;; Buffers can't be input by name, so if the default value is a buffer, just use it.
;; TODO: Find a way to fix this.
org-ql-view-buffers-files
(org-ql-view--expand-buffers-files
(completing-read "Buffers/Files: "
(list 'buffer 'org-agenda-files 'org-directory 'all)
;; nil nil (initial-input))))))
nil nil nil nil (initial-input)))))) ;; (initial-input) is passed to DEF |
|
What I mean is, this is a matter of converting values to and from readable representations--it's a two-way conversion. The variable in question may have a few different forms as its value, and each possible form must be printed and read again from the user's input. You seem to be reporting a problem with one of those forms and trying to fix it for that form. What about the other forms? Using a different argument to |
Handles the different values `org-ql-view-buffers-files` can hold. Use completing-read only if `org-ql-view-buffers-files` is nil or the contracted form of `org-ql-view-buffers-files` is a string.
|
I agree with your second point, that might not be ideal. As for the different conditions, it seems I've misunderstood what how the function in question works. :) Am I correct in assuming the
Also is there a particular reason to wrap the I've pushed a version of |
|
Also while digging into how the above function works, I came across this - the documentation of
when using interactively. Is this supported? Testing it doesn't seem to work? I tried the following for "Buffer/files:"
|
The function exists because, e.g. if the user is searching the list of files in the variable I wasn't aware of any issue with
In order to fix this bug without potentially causing any others, I think it's necessary add tests for all of its intended input and output forms. If we don't enumerate and test them, we're likely to overlook some.
What happens? Anyway, try enclosing them in quotes. If that still doesn't work, then it's probably a bug; I never pass buffer names interactively, so it's not something I've tested. (This is an example of why it needs comprehensive tests.) |
I have 4 different version installed across different pc's I use :') The issue happens with calling (org-ql-search '("~/Documents/orgfile1.org" "~/Documents/orgfile1.org") query)The query would run and I can get the view. I just started looking into writing test cases in emacs. When I have some time, I'll try to write a few cases to test the expand and contract behaviour for the different conditions as well as for the To sum up, is the behavior of
I also opened a new issue for the space-seperated buffers/files issue #230. |
Does it also happen if you use the Transient dispatcher to modify the buffers/files list? i.e. press
Ok, thanks. Please see
No, I don't think that's correct. Please see the function's definition; it's very short. I think it's important for us to keep in mind that there are multiple functions interacting here: |
Yep, same behavior. I had pushed a set of test for the |
|
I'm impressed! You learn and work quickly. You seem to have made good use of Buttercup's features. I haven't read every line of your tests yet, but it looks like you were thorough. Note that buffers are not Also, to be clear, the completion is not intended to actually offer completion of buffer or file names; it's just intended to guide the user in choosing the right kinds of values to this argument. (We could consider enhancing the completion for buffer/file names in the future.) What do you think? Thanks very much for your work on this. You've saved me a lot of time. :) |
|
You may be in the middle of working on this now, but I just happened to see this, and maybe it would be good to let you know now: I understand that factoring out that code into a function helps with testing, but I'd prefer not to do that unless it's really necessary. I don't think we need to do that to test the behavior of these other functions. |
|
By the way, I've had second thoughts about mocking functions' return values. I think we should generally avoid that, because it could mask problems if those functions change (and sometimes Org functions do). Instead, we can bind the variables that affect those functions' return values. (I'm making this comment here rather than as a review comment in each place in the code; sorry for any confusion.) |
|
Anytime man. I enjoy hacking stuff, so :D Regarding reading buffers, what you suggest makes sense. Following your suggestion, I have added the following:
This still leaves the issue of how to process lists in the view. One potential solution is to define a separator (eg: ";") and allow typing in buffer-names/file-name separated by it. Is there a better way to do that? |
|
Just saw the messages. I have a habit of abstracting whenever possible :) |
|
Also, in the last commit, I am dumping the default value of the contracted |
Fn: org-ql-view--complete-buffers-files With buffers, org-ql-view--contract-buffers-files return the buffer name. When org-ql-view-buffers-files is a list, just dumpt it as a string and check if completion-read returns the same value, if so return the original value of org-ql-view-buffers-files
Please give specific examples so we can talk about this concretely and try to devise a solution. We might need to use
I'd prefer not to use separators other than spaces, and filenames with spaces should be quoted. Maybe we should separate this work into two phases:
We should do phase 1 first so that the next stable release of org-ql can be made without this bug. Then we can consider phase 2 for future improvements. |
I agree, but I am having a hard time separating these two: To be clear, as I described before, the problem is when we have I feel like the having the list represented as a space (or any other character (maybe make this a customizable value?)) separated string might be a better idea? What do you think? On the same note, similar to my above comment, perhaps raise |
Filenames may contain spaces, so we have two choices: 1) quote every filename, which would be more tedious for the user; 2) only quote filenames with spaces, which would probably require either some kind of fancy parsing or calling
I think that would be confusing for the user: a query could work when calling Anyway, I think I'll plan for this issue/PR to be addressed/merged in 0.7, that way it won't hold up 0.6, which has had this issue for a while already without many users noticing. |
I'm not sure what you mean.
I haven't used completing-read-multiple much, so I don't know how well I like it. But I think it's worth testing, yes. |
I meant the validation of the input. In retrospect, I think it might not be needed at all, as the values are processed by I'll push a version that uses |
|
@alphapapa would you be able to try the latest commit on this PR and see how the UI feels? It uses the If this is a satisfactory solution, i'll amend the test cases as well |
|
Thanks, I think this will work pretty well. A little feedback: here's another way to write this function that I think is a bit clearer: (defun org-ql-view--expand-buffers-files (buffers-files)
"Return BUFFERS-FILES expanded to a list of files or buffers.
The counterpart to `org-ql-view--contract-buffers-files'."
(--> (-list buffers-files)
(pcase-exhaustive it
("all" (--select (equal (buffer-local-value 'major-mode it) 'org-mode)
(buffer-list)))
("org-agenda-files" (org-agenda-files))
("org-directory" (org-ql-search-directories-files))
((or "" "buffer") (current-buffer))
((pred bufferp) (list it))
((pred listp) (list it))
;; A single filename.
((pred stringp) (list it)))
-non-nil -uniq -flatten)) |
|
Finally got around to wrapping this up. |
| :to-throw 'error '("CAUTION: Link not opened because unsafe buffers-files parameter detected: (lambda nil (error UNSAFE))"))) | ||
| :to-throw 'error '("Value lambda is not a valid buffer/file"))) | ||
| (it "Errors for an unquoted lambda" | ||
| (expect (open-link unquoted-lambda-link) | ||
| :to-throw 'error '("CAUTION: Link not opened because unsafe buffers-files parameter detected: (lambda nil (error UNSAFE))"))) | ||
| :to-throw 'error '("Value lambda is not a valid buffer/file"))) | ||
| (it "Errors for a quoted lambda in a list" | ||
| (if (version< (org-version) "9.3") | ||
| (expect (open-link quoted-lambda-in-list-link) | ||
| :to-throw 'error '("CAUTION: Link not opened because unsafe buffers-files parameter detected: ((quote (lambda nil (error UNSAFE))))")) | ||
| :to-throw 'error '("Value (quote (lambda nil (error UNSAFE))) is not a valid buffer/file")) | ||
| (expect (open-link quoted-lambda-in-list-link) | ||
| :to-throw 'error '("CAUTION: Link not opened because unsafe buffers-files parameter detected: ('(lambda nil (error UNSAFE)))")))) | ||
| :to-throw 'error '("Value (quote (lambda nil (error UNSAFE))) is not a valid buffer/file")))) | ||
| (it "Errors for an unquoted lambda in a list" | ||
| (expect (open-link unquoted-lambda-in-list-link) | ||
| :to-throw 'error '("CAUTION: Link not opened because unsafe buffers-files parameter detected: ((lambda nil (error UNSAFE)))")))) | ||
| :to-throw 'error '("Value (lambda nil (error UNSAFE)) is not a valid buffer/file")))) |
There was a problem hiding this comment.
Why did you change these errors? These are important safety tests.
There was a problem hiding this comment.
I had added a explicit error when the input to the org-ql-view--expand-buffers-files function,
Line 1108 in 564d491
According to my understanding, the previous errors being tested were because of the pcase failing. So roll back the error I triggering and the pacse to fail which would lead to the older errors?
Even then, in this implementation the return value is always a list, which makes the error being produced to be a bit different than what it previously had.
There was a problem hiding this comment.
I'm not sure what you mean. Do you understand the potential security issue here?
There was a problem hiding this comment.
To clarify, the potential values for the org-ql-view--expand-buffers-files now are the following:
- A buffer
- A string
- A file-name
- one of "all", "org-agenda-files", "org-directory", "buffer", or an empty string (which evaluates to the same values as "buffer")
- a list containing above values
I am also converting all buffer objects to string values to avoid duplicates that are introduced because of the buffer/buffer-name of a file and a file-name being passed.
As a result this function now always returns a list of strings (or at least should, will add tests for these :D).
This is in part due to moving to using completing-read-multiple which returns a list by default. Because of this, I had dropped the listp predicate in the pcase inside the function since now all elements in the list are being processed individually. This in-turn results in the unsafe values for buffers/files being tested in the link safety section never getting to the point where that error gets signalled since they never match any of the values in the current pcase. Which was why I was signalling an error when an invalid value is passed. Does this make sense?
There was a problem hiding this comment.
I may have overlooked that functions are a valid input to buffers-files in org-ql-select. Which might be creating a whole bunch of new problems :)
Currently (in the master branch) the following should create a view with the function
(org-ql-search #'a-function-returning-list-of-files '(level 1))with the value for org-ql-view-buffers-files is set to #'a-function-returning-list-of-files.
If I understand this correctly, this is left as such so refreshing the view would recall the function? (or alternatively the evaluated value can be stored?)
I am not sure how a function would translate to a value in completing-read-mulitple, so I am skipping prompting for that entirely when refreshing a view with a function as a value for org-ql-view-buffers-files.
The safety concern you were referring to before comes from allowing a function to be used as a parameter for org-ql-select? Currently the org-ql-view--expand-buffers-files function would signal an error if used with a function, and when storing a link, the function's evaled value is used for buffers-files.
Does that cover the problem with unsafe values for buffers-files?
There was a problem hiding this comment.
You can see where the error is signaled here:
Line 638 in 94f9e6f
That safety check should be left there, in that function, where links are followed. IIUC, the change you made to the tests would cause that check to not be tested, which could lead to security vulnerabilities in the future.
There was a problem hiding this comment.
That check looks at either the value returned by the org-ql-view--expand-buffers-files or the current-buffer. If I understand it correctly, it was there to make sure the value returned by the org-ql-view--expand-buffers-files function is not another function that can get evaluated once passed to org-ql-select. Currently, if I understand when and how org-ql-view--expand-buffers-files gets called, in a valid situation, it only should get values that I had previously mentioned, anything else would result in the error on the new tests being signaled.
If the concern is future changes to the org-ql-view--expand-buffers-files function results in returning a function value, that function can be mocked to make sure the error in
Line 638 in 94f9e6f
get's signaled?
|
BTW, it would be easier for me to review the code and the changes made in response if you left the conversations "unresolved" until the changes are actually made. When I come back to a large change like this after a while, it takes some time to reacquaint myself with it and what I said about it before. |
|
@ahmed-shariff If I may be frank, every time I revisit this PR, I feel lost. Reading all of the discussion again, it's still hard to understand exactly what this PR is intended to address and how it does. It's probably my fault for not defining the problems more clearly in the first place before giving feedback on code. I wish that I could just merge the code and deal with any minor fallout later, but given the potential security issues (even though they might be downstream from this PR's code), I don't feel like that would be the responsible thing to do. I realize that you've put a lot of work into this patch, and that you've been very patient with my requested changes, so I don't want to discard your work. Would you be willing to consider starting this process over, defining the problems more explicitly in an issue, then addressing them in one or more new PRs, which could reuse the code from this one? I think this is a case where some degree of test-driven development would be helpful--at least, for me, as I try to understand the problems we're trying to solve and exactly how we're trying to do it. For example, I'd like to first have the problems clearly described in one or more issues, then have a PR for each one, each of which should start by adding a failing test that should be fixed by later commits. I'm trying to develop this package in a more organized way to ensure that it remains reliable and robust, but that does sometimes mean more overhead for contributions. As well, since I work on this in spare time, it sometimes takes a while to review and integrate patches. Let me know what you think. Thanks. |
|
I totally understand. Ill close this PR and start anew. |
|
if it's any consolation, I also need a few minutes every time I revisit this XD |
|
Thanks. |
Fix #227